Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit f48f507. ± Comparison against base commit 2ba0302.

♻️ This comment has been updated with latest results.

The except ValueError makes me a little nervous as this could catch an unrelated error and give the wrong message. Would

cell_origins = _read_entry(f, int_type)
if isinstance(cell_origins, int):
    raise ValueError('Old castep file detected: '
                     'Euphonic only supports post-Castep 17.1 files. '
                     'Please rerun the calculation with a newer version '
                     'of Castep with the original .cell file and a '
                     '.castep file with a single line with the '
                     '"continuation: <old.castep_bin>" keyword and '
                     'use the new output .castep_bin file in Euphonic.')
cell_origins = np.reshape(cell_origins, (n_cells_in_sc, 3))

be a bit safer?

Ok, have pushed that up. It would be helpful to have a small reference file so we can test that the right error is raised. @krefson would you happen to have a very small and old .castep_bin file with force constants we can add to the test set?

The unit test failures seem to be spglib-related, probably there was a breaking API change? I'll take a look.

Thanks Adam! I was going to try to look into the test failure - it doesn't seem to be related to this (I get the same errors on master) - but I guess that should probably go into a different PR...

The dump files contain a version number, and I suggest checking against that.

The checkpoint version is encoded as a floating-point number as ASCII in a character*10 field in the record immediately following BEGIN_PARAMETERS_DUMP.

The format change was introduced in CASTEP 17.x, so testing for < 17.0 and reporting "not supported" should work as a robust safeguard.

If you really wanted to read dump files with version <= 16.x, then you will indeed need to recompute the supercell_origins vector. There is a short Fortran routine cell.f90 named cell_generate_supercell_origins which would be easy to transliterate into Python.

I don't think we need to support CASTEP versions < 17, especially if an external workaround is available.

I'll implement the version number check, that seems a better way of handling things.

Ok, I am using packaging.version to compare the version numbers. This adds a dependency, unfortunately, but

• it is quite a common one that replaces some things that were in standard library
• (e.g. it is also required by matplotlib, sphinx, black and ipython)
• it makes a rather ugly comparison operation very clean and easy 😅

I think we should wait until #298 is merged and rebase this branch in order to get a clean error check before a final review/merge.

We can also achieve that from this PR with the "squash and merge" option. I'll add a changelog line first, though.